Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

UI: Fix for group icon #7552

Merged
merged 3 commits into from
Mar 20, 2021
Merged

UI: Fix for group icon #7552

merged 3 commits into from
Mar 20, 2021

Conversation

mohit038-zz
Copy link
Contributor

@mohit038-zz mohit038-zz commented Mar 20, 2021 •
edited
Loading

Fixes #4129

Replaced the default group play icon by small circle.

Screenshot 2021-03-20 at 4 13 02 PM

I think this also fixes #7550.

Screenshot 2021-03-20 at 4 59 33 PM

Screenshot 2021-03-20 at 5 07 50 PM

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@tobiasdiez
Copy link
Member

Thanks for your PR. The problem with the default circle is that it is rather big and thus dominates the interface too much (it's pretty hard to read the group label because your eyes are constantly focusing the big colorfull blob in front of it). It would be thus nice if this circle could be scaled down (without scaling other icons, because they get too small otherwise).

@Siedlerchr
Copy link
Member

Can you check if the color thing fixes #7550 as well?

@mohit038-zz
Copy link
Contributor Author

@tobiasdiez
Thanks for the suggestion, I think the icon library has been updated and different sizes of circles are already present in the library.
Let me know which one do you prefer.

  1. CIRCLE_MEDIUM

Screenshot 2021-03-20 at 6 37 23 PM

  1. RECORD

Screenshot 2021-03-20 at 6 56 09 PM

@mohit038-zz
Copy link
Contributor Author

mohit038-zz commented Mar 20, 2021 •
edited
Loading

Can you check if the color thing fixes #7550 as well?

I checked it and also updated the screenshots in #7552 (comment).
It fixes all issues mentioned in #7550.

@Siedlerchr
Copy link
Member

Cool! For me RECORD looks appropriate

@tobiasdiez
Copy link
Member

That's awesome. I agree, record looks the best.

tobiasdiez
tobiasdiez reviewed Mar 20, 2021
View reviewed changes
src/main/java/org/jabref/gui/icon/InternalMaterialDesignIcon.java Outdated
@@ -41,7 +43,7 @@ public Node getGraphicNode() {
Ikon icon = icons.get(0);
FontIcon fontIcon = FontIcon.of(icon);
fontIcon.getStyleClass().add("glyph-icon");
color.ifPresent(fontIcon::setIconColor);
color.ifPresent(color -> fontIcon.setStyle(fontIcon.getStyle() + String.format("-fx-fill: %s;", ColorUtil.toRGBCode(color))));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why setIconColor doesn't work? Maybe add a short comment so that we will not revert it in the future by accident.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I have no idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has something to do with the style class. If you uncomment the line "getStyleClass.add("glyph-icon")" than it works properly. However, then the rest of the icons are black because that property is set in the css (and is obviously different for dark and base.css) and therefore overrides the color of the 'FontIcon".

So add a comment like:
Override the default color from the css files

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 20, 2021
@tp-1000 tp-1000 mentioned this pull request Mar 20, 2021
Closed
1 task
@tobiasdiez
Copy link
Member

Thanks again!

@tobiasdiez tobiasdiez merged commit d31ef4d into JabRef:master Mar 20, 2021
@mohit038-zz mohit038-zz deleted the fix-issue-4129 branch March 20, 2021 23:47
Siedlerchr added a commit that referenced this pull request Mar 28, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into writebackupToTemp
f99ba6f 
* upstream/master: (191 commits)
  Fix for issue 7416: font size of the preferences dialog does not update with the rest of the GUI. (#7509)
  Fix school/instituation is printed twice (#7574)
  Dsiable notarisation until we hae an account for JabRef e.V. (#7572)
  Fix citation keys unintentionally being overwritten on import (#7443)
  Fix AuthentificationPlugin not declared in mergedModule (#7570)
  Suggestions for changes in caching latex free authors (#7301)
  Add simple Unit Tests (#7542)
  Fix drag and drop into empty library (#7555)
  Bump richtextfx from 0.10.4 to 0.10.6 (#7563)
  Bump pdfbox from 2.0.22 to 2.0.23 (#7561)
  Bump org.eclipse.jgit (#7560)
  Bump fontbox from 2.0.22 to 2.0.23 (#7562)
  Bump guava from 30.1-jre to 30.1.1-jre (#7564)
  Bump xmpbox from 2.0.22 to 2.0.23 (#7565)
  Bump hmarr/auto-approve-action from v2.0.0 to v2.1.0 (#7566)
  Add gource (#7193)
  UI: Fix for group icon (#7552)
  Fix for issue 6487: Opening BibTex file (doubleclick) from Folder with spaces not working (#7551)
  add ability to insert arxivId (#7549)
  Fixed missing trigger for linked file operations (#7548)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasdiez tobiasdiez tobiasdiez left review comments

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Monocolor for icons Replace default group icon by small circle
4 participants
@mohit038-zz @tobiasdiez @Siedlerchr @mohit038